ci(cppcheck-all, cppcheck-differential): add cppcheck #7262

kminoda · 2024-06-04T06:57:35Z

Description

Add cppcheck for GitHub Actions

cppcheck-differential: A workflow that is applied only to the pull request, mainly to check if the modified files are cppcheck-error-free. This only checks limited items in cppcheck to make it pass in the current codebase, aiming at setting this CI mandatory ASAP.
cppcheck-all: A workflow that is intended to run on all files under Autoware Universe, mainly to monitor the remaining cppcheck errors. This may also be run manually as well.

Future improvements would include

Address the errors pointed out in cppcheck-all to achieve cppcheck-error-free Autoware Universe
Apply cppcheck not only to Autoware Universe but also to other implementations (e.g. autoware_common)
Summarize the results of cppcheck-all to motivate each engineers to address the warnings

Tests performed

See the results of GitHub Actions

Notes for reviewers

We would eventually like to apply most of the important cppcheck items, but for now I created a huge .cppcheck_suppression file to suppress all the errors.

Interface changes

None

ROS Topic Changes

None

ROS Parameter Changes

None

Effects on system behavior

None

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

I've confirmed the contribution guidelines.
The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

The PR follows the pull request guidelines.
The PR has been properly tested.
The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

There are no open discussions or they are tracked via tickets.
The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: kminoda <[email protected]>

kminoda · 2024-06-04T12:29:20Z

@veqcc @TakaHoribe Hi, would you review this PR?

veqcc · 2024-06-04T12:38:20Z

@kminoda
Thank you for your quick actions!!

My first question is, are all the features listed in .cppcheck_suppressions expected to pass immediately? Or, will be mandatory in the near future, like in several months?

If you are willing to make it mandatory as soon as possible, I think we should choose the features more carefully.
In other words, what happens if someone makes a change on a file which already has cppcheck warnings.

kminoda · 2024-06-04T12:54:36Z

@veqcc Hi, thank you for the comment!

.cppcheck_suppressions is the list of items to "ignore" rather than to "include". So no, those features are not expected to pass immediately.

The idea behind this is to suppress all (and only) the errors in the latest Autoware Universe so that we can make the CI mandatory as soon as this PR is merged. The list contains all the warning items in the latest Autoware Universe, so at least we can avoid an unexpected additional warning from cppcheck.

(BTW the only risk I can see is the case when cppcheck raises false-positive errors. However, we can address this risk by either adding it in .cppcheck_suppressions or introduce inline suppression as in here, so don't recognize this as a blocking factor)

kminoda · 2024-06-04T13:10:38Z

@mitsudome-r Hi, I would like to make this cppcheck-differential required after this PR is merged if you don't see any concerns. Let me know if you have any comments. Thanks!

veqcc · 2024-06-04T14:09:08Z

@kminoda

.cppcheck_suppressions is the list of items to "ignore" rather than to "include". So no, those features are not expected to pass immediately.

I was misunderstanding in an opposite way, thanks!

(BTW the only risk I can see is the case when cppcheck raises false-positive errors.

Static analysis tools are always incomplete.
Most documents say that several tools are recommended to be used at the same time.

I will approve it later.
My final concern is, is there any future plan which features listed in the suppression file are to be mandatory some time or suppressed forever?

kminoda · 2024-06-04T14:35:28Z

@veqcc Yes, this PR is merely an initial step. The next action would be to reduce the suppression items one by one. FYI you can check the current error statistics in cppcheck-all CI result e.g. here. This wpuld be helpful to discuss which items we should address as a next step.

Signed-off-by: kminoda <[email protected]>

TakaHoribe

The idea behind this is to suppress all (and only) the errors in the latest Autoware Universe so that we can make the CI mandatory as soon as this PR is merged.

This sounds reasonable to me. I approve this, but please check one question below.

.github/workflows/cppcheck-all.yaml

…tion#7262) * ci: add cppcheck Signed-off-by: kminoda <[email protected]> * add more suppression Signed-off-by: kminoda <[email protected]> * update cppcheck version Signed-off-by: kminoda <[email protected]> * update suppressions list Signed-off-by: kminoda <[email protected]> * add two workflows Signed-off-by: kminoda <[email protected]> * change name Signed-off-by: kminoda <[email protected]> * change timing of all ci Signed-off-by: kminoda <[email protected]> * update Signed-off-by: kminoda <[email protected]> * update Signed-off-by: kminoda <[email protected]> * update Signed-off-by: kminoda <[email protected]> * fix Signed-off-by: kminoda <[email protected]> * update name Signed-off-by: kminoda <[email protected]> * fix mistake Signed-off-by: kminoda <[email protected]> * add echo in diff Signed-off-by: kminoda <[email protected]> * update regex Signed-off-by: kminoda <[email protected]> * add statistics for cppcheck-all Signed-off-by: kminoda <[email protected]> * avoid checking cppcheck dir Signed-off-by: kminoda <[email protected]> * error-exit Signed-off-by: kminoda <[email protected]> * dummy test Signed-off-by: kminoda <[email protected]> * upload file even if it fails Signed-off-by: kminoda <[email protected]> * fix bug Signed-off-by: kminoda <[email protected]> * revert unnecessary commit Signed-off-by: kminoda <[email protected]> * minor fix Signed-off-by: kminoda <[email protected]> * dummy test Signed-off-by: kminoda <[email protected]> * dummy test Signed-off-by: kminoda <[email protected]> * modify to show cppcheck result even when failure Signed-off-by: kminoda <[email protected]> * finalize PR Signed-off-by: kminoda <[email protected]> * fix pre-commit Signed-off-by: kminoda <[email protected]> * fix pre-commit Signed-off-by: kminoda <[email protected]> --------- Signed-off-by: kminoda <[email protected]>

* ci: add cppcheck Signed-off-by: kminoda <[email protected]> * add more suppression Signed-off-by: kminoda <[email protected]> * update cppcheck version Signed-off-by: kminoda <[email protected]> * update suppressions list Signed-off-by: kminoda <[email protected]> * add two workflows Signed-off-by: kminoda <[email protected]> * change name Signed-off-by: kminoda <[email protected]> * change timing of all ci Signed-off-by: kminoda <[email protected]> * update Signed-off-by: kminoda <[email protected]> * update Signed-off-by: kminoda <[email protected]> * update Signed-off-by: kminoda <[email protected]> * fix Signed-off-by: kminoda <[email protected]> * update name Signed-off-by: kminoda <[email protected]> * fix mistake Signed-off-by: kminoda <[email protected]> * add echo in diff Signed-off-by: kminoda <[email protected]> * update regex Signed-off-by: kminoda <[email protected]> * add statistics for cppcheck-all Signed-off-by: kminoda <[email protected]> * avoid checking cppcheck dir Signed-off-by: kminoda <[email protected]> * error-exit Signed-off-by: kminoda <[email protected]> * dummy test Signed-off-by: kminoda <[email protected]> * upload file even if it fails Signed-off-by: kminoda <[email protected]> * fix bug Signed-off-by: kminoda <[email protected]> * revert unnecessary commit Signed-off-by: kminoda <[email protected]> * minor fix Signed-off-by: kminoda <[email protected]> * dummy test Signed-off-by: kminoda <[email protected]> * dummy test Signed-off-by: kminoda <[email protected]> * modify to show cppcheck result even when failure Signed-off-by: kminoda <[email protected]> * finalize PR Signed-off-by: kminoda <[email protected]> * fix pre-commit Signed-off-by: kminoda <[email protected]> * fix pre-commit Signed-off-by: kminoda <[email protected]> --------- Signed-off-by: kminoda <[email protected]>

kminoda added 7 commits June 4, 2024 15:45

ci: add cppcheck

8b8de24

Signed-off-by: kminoda <[email protected]>

add more suppression

bf154e8

Signed-off-by: kminoda <[email protected]>

update cppcheck version

e57410b

Signed-off-by: kminoda <[email protected]>

update suppressions list

2e2cb23

Signed-off-by: kminoda <[email protected]>

add two workflows

e7064d5

Signed-off-by: kminoda <[email protected]>

change name

bd4b78e

Signed-off-by: kminoda <[email protected]>

change timing of all ci

01739e2

Signed-off-by: kminoda <[email protected]>

github-actions bot added the type:ci Continuous Integration (CI) processes and testing. (auto-assigned) label Jun 4, 2024

kminoda added 8 commits June 4, 2024 15:58

update

d50d05d

Signed-off-by: kminoda <[email protected]>

update

de6f3e8

Signed-off-by: kminoda <[email protected]>

update

a89e8b6

Signed-off-by: kminoda <[email protected]>

fix

85c5036

Signed-off-by: kminoda <[email protected]>

update name

4eb1d9c

Signed-off-by: kminoda <[email protected]>

fix mistake

e715115

Signed-off-by: kminoda <[email protected]>

add echo in diff

ed14e35

Signed-off-by: kminoda <[email protected]>

update regex

9231644

Signed-off-by: kminoda <[email protected]>

github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Jun 4, 2024

kminoda force-pushed the ci/add_cpp_check branch from 0086851 to 0e77dba Compare June 4, 2024 07:29

github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) and removed component:localization Vehicle's position determination in its environment. (auto-assigned) labels Jun 4, 2024

kminoda force-pushed the ci/add_cpp_check branch from 0e77dba to 4427fd6 Compare June 4, 2024 07:32

github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) and removed component:perception Advanced sensor data processing and environment understanding. (auto-assigned) labels Jun 4, 2024

add statistics for cppcheck-all

4f10212

Signed-off-by: kminoda <[email protected]>

kminoda force-pushed the ci/add_cpp_check branch from 4abb0ff to 4f10212 Compare June 4, 2024 07:41

github-actions bot removed the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jun 4, 2024

avoid checking cppcheck dir

2d3bbb3

Signed-off-by: kminoda <[email protected]>

kminoda force-pushed the ci/add_cpp_check branch from b05b15c to 2d3bbb3 Compare June 4, 2024 08:08

kminoda added 2 commits June 4, 2024 20:57

error-exit

ba13741

Signed-off-by: kminoda <[email protected]>

dummy test

e32bc67

Signed-off-by: kminoda <[email protected]>

kminoda added 4 commits June 4, 2024 21:04

fix bug

f64238b

Signed-off-by: kminoda <[email protected]>

revert unnecessary commit

c8f59cb

Signed-off-by: kminoda <[email protected]>

minor fix

7923ce9

Signed-off-by: kminoda <[email protected]>

dummy test

7a02961

Signed-off-by: kminoda <[email protected]>

github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jun 4, 2024

kminoda added 3 commits June 4, 2024 21:13

dummy test

8c7d41c

Signed-off-by: kminoda <[email protected]>

modify to show cppcheck result even when failure

330ceb8

Signed-off-by: kminoda <[email protected]>

finalize PR

b8829a4

Signed-off-by: kminoda <[email protected]>

github-actions bot removed the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jun 4, 2024

kminoda changed the title ~~ci: add cppcheck~~ ci(cppcheck-all, cppcheck-differential): add cppcheck Jun 4, 2024

kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jun 4, 2024

kminoda requested review from TakaHoribe and veqcc June 4, 2024 12:29

kminoda marked this pull request as ready for review June 4, 2024 12:29

veqcc approved these changes Jun 4, 2024

View reviewed changes

kminoda and others added 4 commits June 4, 2024 23:39

Merge branch 'main' into ci/add_cpp_check

3384a55

fix pre-commit

ac26f55

Signed-off-by: kminoda <[email protected]>

fix pre-commit

6948f0b

Signed-off-by: kminoda <[email protected]>

Merge branch 'main' into ci/add_cpp_check

a8a59f8

TakaHoribe approved these changes Jun 5, 2024

View reviewed changes

.github/workflows/cppcheck-all.yaml Show resolved Hide resolved

Merge branch 'main' into ci/add_cpp_check

9890b2e

kminoda merged commit 0160f36 into autowarefoundation:main Jun 5, 2024
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(cppcheck-all, cppcheck-differential): add cppcheck #7262

ci(cppcheck-all, cppcheck-differential): add cppcheck #7262

kminoda commented Jun 4, 2024 •

edited by TakaHoribe

Loading

kminoda commented Jun 4, 2024

veqcc commented Jun 4, 2024 •

edited

Loading

kminoda commented Jun 4, 2024 •

edited

Loading

kminoda commented Jun 4, 2024

veqcc commented Jun 4, 2024 •

edited

Loading

kminoda commented Jun 4, 2024 •

edited

Loading

TakaHoribe left a comment

ci(cppcheck-all, cppcheck-differential): add cppcheck #7262

ci(cppcheck-all, cppcheck-differential): add cppcheck #7262

Conversation

kminoda commented Jun 4, 2024 • edited by TakaHoribe Loading

Description

Related links

Tests performed

Notes for reviewers

Interface changes

ROS Topic Changes

ROS Parameter Changes

Effects on system behavior

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

Post-review checklist for the PR author

kminoda commented Jun 4, 2024

veqcc commented Jun 4, 2024 • edited Loading

kminoda commented Jun 4, 2024 • edited Loading

kminoda commented Jun 4, 2024

veqcc commented Jun 4, 2024 • edited Loading

kminoda commented Jun 4, 2024 • edited Loading

TakaHoribe left a comment

Choose a reason for hiding this comment

kminoda commented Jun 4, 2024 •

edited by TakaHoribe

Loading

veqcc commented Jun 4, 2024 •

edited

Loading

kminoda commented Jun 4, 2024 •

edited

Loading

veqcc commented Jun 4, 2024 •

edited

Loading

kminoda commented Jun 4, 2024 •

edited

Loading